Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

luci-base: add member allowduplicates to DynamicList #7558

Merged
merged 1 commit into from
Jan 22, 2025

Conversation

muink
Copy link
Contributor

@muink muink commented Jan 11, 2025

  • This PR is not from my main or master branch 💩, but a separate branch ✅
  • Each commit has a valid ✒️ Signed-off-by: <[email protected]> row (via git commit --signoff)
  • Each commit and PR title has a valid 📝 <package name>: title first line subject for packages
  • Incremented 🆙 any PKG_VERSION in the Makefile
  • Tested on: (x86_64, 24.10.0-rc4, Chrome) ✅
  • ( Preferred ) Mention: @systemcrash
  • ( Preferred ) Screenshot or mp4 of changes:
  • ( Optional ) Closes: e.g. openwrt/luci#issue-number
  • ( Optional ) Depends on: e.g. openwrt/packages#pr-number in sister repo
  • Description: (describe the changes proposed in this PR)

Description:

Add member allowduplicates to DynamicList to allow duplicate underlying form values.

allowduplicates defaults is null, DynamicList will keep the same behavior as before.
If o.allowduplicates = true, the underlying form value will not be checked for duplication.

Screenshot:

image

@systemcrash
Copy link
Contributor

Although it might be a solution in general, it might go unused but for your use-case @muink. Perhaps there is a better way to capture config?

This seems OK in general. Opinions from others? @hnyman @stokito @dannil ?

@muink
Copy link
Contributor Author

muink commented Jan 13, 2025

I've tried a few other solutions, but they are all too heavy. This is currently the simplest stupidst solution I can come up with.
And uci can store duplicate lists, it would be strange if luci can't do that.

@stokito
Copy link
Contributor

stokito commented Jan 13, 2025

The change is small and seems reasonable. Maybe we can find other places were the duplicate property is needed? Just to have a better understanding. Because now I think that we can just remove the check for a duplicates, or show a confirmation to a user when adding a duplicate.

@systemcrash The word "duplicate" is an action so maybe call it "duplicates" or even "allowDuplicates" or "duplication"?

@dannil
Copy link
Contributor

dannil commented Jan 13, 2025

This seems OK in general. Opinions from others? @hnyman @stokito @dannil ?

I'm fine with it, the semantics of a list is to allow duplicates anyway, otherwise you're dealing with sets, and since it doesn't break existing default functionality I don't see any problem.

@stokito stokito mentioned this pull request Jan 13, 2025
@systemcrash
Copy link
Contributor

systemcrash commented Jan 14, 2025

Hi @muink - please rename the setting to allowduplicates. I like @jow-'s suggestion from the linked ticket (and @stokito's above), and it's what I would settle on also, since the name makes clear what the boolean value hinges on (and that duplicates is a plural noun).

@muink
Copy link
Contributor Author

muink commented Jan 14, 2025

force pushed.

@muink muink changed the title luci-base: add member duplicate to DynamicList luci-base: add member allowduplicates to DynamicList Jan 14, 2025
Add member `allowduplicates` to `DynamicList` to allow duplicate
underlying form values.

`allowduplicates` defaults is `null`, `DynamicList` will keep the
same behavior as before.
If `true`, the underlying form value will not be checked
for duplication.

Signed-off-by: Anya Lin <[email protected]>
@systemcrash systemcrash merged commit 9d9a7f4 into openwrt:master Jan 22, 2025
5 checks passed
@muink muink deleted the luci-base branch January 23, 2025 00:39
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants